Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Project coverage is 36.53%. Comparing base (95ada0b) to head (82be94c).
Report is 92 commits behind head on master.

@@            Coverage Diff             @@
##           master    #1131      +/-   ##
==========================================
+ Coverage   36.50%   36.53%   +0.02%     
==========================================
  Files         429      432       +3     
  Lines       19120    19222     +102     
  Branches     5653     5667      +14     
==========================================
+ Hits         6980     7023      +43     
- Misses      12102    12161      +59     
  Partials       38       38              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Rather than adding an extra useState to get around the problem, do we know why that intended behaviour is not being achieved by the populateFromUserContext useEffect? Happy to chat through this as it is quite a tricky bit of code!

@mlt47 Digging a little bit deeper, I've found that the crux of the issue seems to be setPopulatedUserContext updating at the wrong times. An empty array is truthy, so searchStages and searchExamBoards can update it to a populated user context state without being the correctly populated values (which is why the extra searchAndUpdateURL() I added was needed as a workaround in the previous solution above)
(I was looking at the wrong variables here, but the core idea still applies. We rely on populatedUserContext to tell us when loading has completed, but it finishes earlier than that before other set...s have applied).

It seems with some quick testing like it can be solved fairly simply by separating setPopulatedUserContext into its own useEffect with dependencies on the actual search terms or something similar (with no need for the extra useState or other work I've done here), but I'll have to experiment with how this works in different contexts to confirm this.

This now boiled down to two main changes:
1 - Changing the check for an array of pre-existing filters (as obtained from the URL parameters) into a check for the existence of those URL parameters directly.

This is needed so an initial 'default' user state doesn't take precedence over the real user context, but URL parameters do in order to allow the page to be refreshed/copied/etc.

2 - Making the searchAndUpdateURL() useEffect have the search parameters as dependencies so it continues to be checked later during the loading phase, conditional on the search parameters matching those from user context.

This is needed so searchAndUpdateURL() is not called early due to userContext being loaded in without React finishing processing the setStages and setExamBoards functions. We know the set... functions have finished when the parameters successfully match.

@mlt47 I think your change makes a lot of sense. Only once a user object is instantiated will they will be logged in, at which point the object is also instantiated enough to include a registeredContext. It works very simply and cleanly most of the time.

However, I think this solution fails to cover some of the edge cases:

• Most notably, currently a logged out user on Ada will have default preferences applied on page-load, so make a search for all "Ada" Exam Board tagged items (which is all items). This disabled that page load search.
  Physics doesn't have any default logged-out search parameters, so it doesn't change anything there (unless we want to specifically add a set now).
  [James said that we should probably just have nothing for both page loads, in which case this is fine]

• Also somewhat notably, if a user has URL parameters other than examBoards and stages set, their blank examBoards/stages filters will be overwritten by their preferred ones on reload.
  e.g. Difficulty: P1, ExamBoard: {}, Stage: {} -> Difficulty: P1, ExamBoard: AQA, Stage: GCSE
  We should probably make filtersHaveNotBeenSpecifiedByQueryParams check for the existence of any URL search parameters. My solution also had this same problem, but I've just noticed it here.

• Not much of an issue but for the sake of keeping record, a user can have no registered preferred stage context on Physics if they keep ignoring the modal to add one - and therefore will have nothing show up on loading the page. I think this is probably correct unless we did decide to add default Physics search parameters, in which case they should probably also be applied in this case.

• On a similar-ish note for a problem that's mostly pre-existing but is highlighted here, if a Physics user has "All stages" set as their preferred Stage, the meaningless parameter "all" will be automatically set. This makes little practical difference to the searching as no questions are tagged with "all", but it adds an invisible count to the Stage filter and appears in the URL.
  I feel that either we should set "all" as a filter that actually does search for all questions (similar to the "Ada" exam board on Ada) OR simply disallow it from the filters. (This can be its own card)